I was just looking at this as I need resources support, and noticed this doesn't provide any way to determine whether the server even supports resources. The only way to tell with this code would be to attempt to make a resource call (e.g. read_resource) and wait for a failure.

I think this should be better exposed as I would like my application to switch to alternate behavior if the MCP server does not support resources. And would like to do so without having to make a call and see if it fails, and then differentiate whether that failure was because of lack of support, or some other reason.

The MCP spec says that the resources capability is used to advertise resources support. However from looking at the code, only serverInfo is captured from the initialize result, and the rest of the initialize response (which includes resources support) is thrown away.

https://github.com/fennb/pydantic-ai/blob/b8424d80eeeb46aef3ad8ae49ae29b1ef820c7af/pydantic_ai_slim/pydantic_ai/mcp.py#L375-L378

Another thing I noticed going through the code, is that for EmbeddedResource handling, the metadata is being thrown away. I need to know the URI of the resource.

I'm not sure if addressing this is even in scope of this PR, as ResourceLink handling is also bad, as it automatically fetches the content instead of providing the link. But issue #3099 addresses this. So I'm not sure if this EmbeddedResource metadata loss issue goes here, there, or in a new issue entirely. But figured it would be at least worth mentioning here.

The MCP spec says that the resources capability is used to advertise resources support. However from looking at the code, only serverInfo is captured from the initialize result, and the rest of the initialize response (which includes resources support) is thrown away.

That's a great point (and not something I was looking at in my changes). Capabilities presumably hasn't been an issue up until this point because MCP was only used for tools in Pydantic AI and it was assumed you wouldn't use an MCP server unless it had tool capabilities.

I'm not quite sure the right way to support this. The upstream mcp library has types for each capability, but replicating/proxying these as native types seems quite verbose.

There is an existing pattern, which is how server_info() is handled (as you point out @phemmer), where the details are stored/cached at initalize time and exposed like so:

@property
    def server_info(self) -> mcp_types.Implementation:
        """Access the information send by the MCP server during initialization."""
        if getattr(self, '_server_info', None) is None:
            raise AttributeError(
                f'The `{self.__class__.__name__}.server_info` is only instantiated after initialization.'
            )
        return self._server_info

We could follow this pattern and just return mcp SDK types. Alternatively, since we don't support listChanged anyway, could we maybe just make them a set of ServerCapabilities literals?

Maybe something like:

ServerCapabilities = Literal[
      "experimental",
      "logging",
      "prompts",
      "resources",
      "tools",
      "completions"
  ]

# MCPServer
@property
def capabilities() -> set[ServerCapabilities]:
    ...

Meh, I guess that's kind ugly: if "resources" in server.capabilities(): ... - perhaps the way mcp already works is superior, so we'd end up with something more like: if server.capabilities.resources: ...? I'm not familiar enough to know what would be more idiomatic for Pydantic.

I don't want this PR to drag on any longer than it already has, but this seems like a reasonable/incremental change overall?

What do you think @DouweM / @Kludex ?

I'm not sure if addressing this is even in scope of this PR, as ResourceLink handling is also bad, as it automatically fetches the content instead of providing the link. But issue #3099 addresses this.

Yep yep - do you mind moving this topic across to #3099 ?

So I'm not sure if this EmbeddedResource metadata loss issue goes here, there, or in a new issue entirely.

@phemmer We have an issue for that on already: #2288. Input on what the better behavior would be is much appreciated!

Meh, I guess that's kind ugly: if "resources" in server.capabilities(): ...

@fennb I don't mind it! @Kludex You?

What happens when you try to retrieve resources from a server that doesn't have that capability?

What happens when you try to retrieve resources from a server that doesn't have that capability?

Right now (current state) it would raise an mcp.shared.exceptions.McpError (ie: MCP SDK class error), with the specifics of the error being dependent on server itself.

Arguably we could choose to intercept this and fail-early if the server doesn't have the capability, but I'm not sure it really adds any value? There's also not much of a pattern in the codebase for this that I can tell (given how lightweight exceptions.py is).

I think we can just leave it up to the client application/user to check which server capabilities exist via something like a capabilities() property before they try to use them (particularly given Resources are supposed to be application controlled, not model controlled), yes?

Since "resources are not supported" or "the requested resource doesn't exist" are such common cases users will typically need to handle, I think they should be our own exceptions or the methods could just return [] / None. I'm fine with that as well, you @Kludex?

Since "resources are not supported" or "the requested resource doesn't exist" are such common cases users will typically need to handle, I think they should be our own exceptions or the methods could just return [] / None. I'm fine with that as well, you @Kludex?

I've done some additional testing. The specifics of errors are pretty server-implementation specific. For example, the official python-sdk's FastMCP server doesn't even have the ability to disable Resource capability advertisement and just returns [] if you call list_resources() on it if none are defined.

read_resource() on a non-existent resource raises an McpError with error = ErrorData(code=0, message='Unknown resource: not://here', data=None). Amusingly, this appears to violate the MCP spec, which says that a not-found should be code -32002.

I think returning None for list_resources() if there's no support makes sense/is simplest.

For other errors in things like read_resource() (ultimately McpError), I guess it does make sense to raise native exception types (same as returning our own data types).

I'll take a pass at this direction to review.

I'm fine with that.

This is ready for re-review. I noticed we're consistently getting test failures on a specific subset of test suites, but they really seem unrelated to these changes as far as I can see? Example here.

@Kludex - This is ready for re-review. Changes should be implemented as proposed/discussed here (along with addressing the other prior review comments).

Let me know if there are any concerns/changes desired!

FWIW, the test failure on 3.12 appears unrelated to my changes (though seems to occur reliably).

@fennb Thanks Fenn, I've gone ahead and merged it and we'll see if the error occurs on main (I've also seen it happen somewhere in between flaky and reliably depending on the PR :/)

@fennb Thanks Fenn, I've gone ahead and merged it and we'll see if the error occurs on main (I've also seen it happen somewhere in between flaky and reliably depending on the PR :/)

Thanks @DouweM @Kludex for your help/guidance on this one (and sorry it dragged on a bit). Let me know if there's anything else (related or otherwise) I can help out with. My general plan is to turn my attention to #3099 next.

@fennb Thanks Fenn! I'm also curious about your take on #3339, it's related about how we expose MCP metadata through Pydantic AI objects (if not to the LLM itself).